-
Notifications
You must be signed in to change notification settings - Fork 269
fix(amazonq): Replacing message bus with direct communication #5949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Qodana Community for JVM1 new problem were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
@@ -134,6 +136,11 @@ class AmazonQPanel(val project: Project, private val scope: CoroutineScope) : Di | |||
browser.complete( | |||
Browser(this@AmazonQPanel, webUri, project).also { browserInstance -> | |||
wrapper.setContent(browserInstance.component()) | |||
|
|||
// Register direct callback instead of using message bus | |||
ChatCommunicationManager.getInstance(project).setChatUpdateCallback { message -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably just associate the browser with the communication manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Browser is not in shared package, and cannot import it. I am able to do it using Any
, but not sure if it is a right way to do it.
…kit-jetbrains into lodogga/memoryUsage
try { | ||
chatAsyncResultManager.createRequestId(partialResultToken) | ||
chatAsyncResultManager.getResult(partialResultToken) | ||
handleCancellation(tabId, browser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this ok to delete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this code is introduced when stop conversation needed to be handled on JB side. Now it is not required. So reverting this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also maintain records for partial results cause they are received separately and we mark them done when the result is completed, are we sure that line 599-600 can be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to mirror the cancellation logic here how it's handled in Eclipse. In the bug bash today removing this results in the original issue where:
- Hitting stop button once stops the response, prints "you have stopped.." message, but stop button still shows
- Hitting stop button second time prints"you have stopped.." again, stop button disappears
jk jk
I did not see that this behavior was regression and that the double clicking stop button issue is still present in prod rn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is executed only on stop button, why do we need to store partial results in this case ? Additionally now stop is being handled by FLARE. So we can revert the code. As mentioned in the description the changes in this PR 5765 can be reverted.
try { | ||
chatAsyncResultManager.createRequestId(partialResultToken) | ||
chatAsyncResultManager.getResult(partialResultToken) | ||
handleCancellation(tabId, browser) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also maintain records for partial results cause they are received separately and we mark them done when the result is completed, are we sure that line 599-600 can be deleted?
@@ -40,7 +40,8 @@ class ActionRegistrar { | |||
val params = SendToPromptParams(selection = codeSelection, triggerType = TriggerType.CONTEXT_MENU) | |||
uiMessage = FlareUiMessage(command = SEND_TO_PROMPT, params = params) | |||
} | |||
AsyncChatUiListener.notifyPartialMessageUpdate(project, uiMessage) | |||
ChatCommunicationManager.getInstance(project).notifyUi(uiMessage) | |||
// AsyncChatUiListener.notifyPartialMessageUpdate(project, uiMessage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
Optimize AmazonQ Plugin Memory Usage [DO NOT MERGE]
Problem
AmazonQ plugin memory usage has increased due to:
Solution
Changes
ChatCommunicationManager
AmazonQPanel
to register callback instead of subscribing to message busAmazonQLanguageClientImpl
andActionRegistrar
to use direct communicationChecklist
Before
After
system is idle from 21:00 to 6:00 in second image
License
I confirm that my contribution is made under the terms of the Apache 2.0 license.